sqlite: add StatementSync.prototype.close() and [Symbol.dispose]() - #64232
sqlite: add StatementSync.prototype.close() and [Symbol.dispose]()#64232araujogui wants to merge 4 commits into
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
aduh95
added
semver-minor
|
We are currently finalizing statements when the wrapper is deconstructed after GC. This PR only makes sense if we are going to change this to an explicit "user should finalize statements themselves" model, in which case we should also expose a named |
I think we can support both approaches. I don't see any issue with that. We can either let the GC finalize statements as they are today, or let users finalize them explicitly with a |
araujogui
changed the title
Honestly, I'm not a fan at all of relying on GC timing. It's too unpredictable and can expose GC timing details (which can be problematic on its own). We originally had |
|
That's the case I had in mind. I don't have a problem with the approach, we just need to be very clear to consumers that the contract is changing. |
cb5696c to
9ed6fae
Compare
This extends explicit resource management support to prepared statements, allowing a StatementSync to be deterministically finalized via a `using` declaration, mirroring the existing DatabaseSync and Session dispose methods. Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
Signed-off-by: Guilherme Araújo <arauujogui@gmail.com>
9ed6fae to
7144743
Compare
Renegade334
left a comment
Renegade334
left a comment
There was a problem hiding this comment.
LGTM. As a follow-up, we should update all of the statement examples in sqlite.md to demonstrate explicit teardown.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #64232 +/- ##
==========================================
- Coverage 90.25% 90.24% -0.02%
==========================================
Files 741 741
Lines 241165 241177 +12
Branches 45428 45426 -2
==========================================
- Hits 217667 217643 -24
- Misses 15069 15114 +45
+ Partials 8429 8420 -9
🚀 New features to boost your workflow:
|
github-actions
Bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
This extends explicit resource management support to prepared statements, allowing a StatementSync to be deterministically finalized via a `using` declaration, mirroring the existing DatabaseSync and Session dispose methods. Signed-off-by: Guilherme Araújo <arauujogui@gmail.com> PR-URL: #64232 Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
Signed-off-by: Guilherme Araújo <arauujogui@gmail.com> PR-URL: #64232 Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
|
Landed in 735a09f...51c0d94 |
Statements could only be finalized by garbage collection or by closing the database, so there was no way to release one deterministically and `using stmt = db.prepare(...)` did nothing. close() finalizes the statement and stops the database tracking it. It throws ERR_INVALID_STATE if the statement is already finalized, if it is currently executing, or if it is called from inside an authorizer callback: finalizing a statement whose sqlite3_step() is on the stack is undefined behavior, and sqlite3_finalize() modifies the connection, which SQLite forbids from an authorizer. That second guard is what every other statement method already does, and is an intentional divergence from node:sqlite, which has no authorizer guard at all. Symbol.dispose never throws, so both unsafe cases become no-ops there and the statement is finalized later by GC or database close. Also drops FinalizeStatement() and FinalizedGetter(), which were never registered on the prototype and so were unreachable. Ports nodejs/node#64232.
No description provided.